Conversation
0a359b2 to
aace063
Compare
Honny1
left a comment
There was a problem hiding this comment.
Please add tests for this. Ideally integration tests, since it doesn’t work on your machine with the latest version of Podman.
Yes i will do in the next days. Greetings, |
podman/domain/images_manager.py
Outdated
| """Import a tarball as an image (equivalent of 'podman import'). | ||
|
|
||
| Args: | ||
| source: Path to a tarball (str) or a file-like object opened in binary mode. |
podman/domain/images_manager.py
Outdated
| APIError: when service returns an error. | ||
| """ | ||
| # Check that exactly one of the data or file_path is provided | ||
| if not data and not file_path: |
| self.client.images.import_image(b'data', b'file_path') | ||
|
|
||
| with self.assertRaises(PodmanError): | ||
| self.client.images.import_image(data=b'data', file_path=b'file_path') |
There was a problem hiding this comment.
Why is file_path a bytes literal?
podman/domain/images_manager.py
Outdated
| if file_path: | ||
| # Convert to Path if file_path is a string | ||
| file_path_object = Path(file_path) | ||
| post_data = file_path_object.read_bytes() # Read the tarball file as bytes |
There was a problem hiding this comment.
I would stream this data instead of reading a multiple GB file into memory.
|
Also, please squash your changes into one commit. |
Yes, will do and i will apply your suggestions. |
71692e1 to
eae68f5
Compare
I applied the requested changes, added extra tests for setting message and changes and squashed all changes in one commit. |
Honny1
left a comment
There was a problem hiding this comment.
ruff (legacy alias)......................................................Failed
- hook id: ruff
- exit code: 1
UP035 `typing.List` is deprecated, use `list` instead
--> podman/domain/images_manager.py:9:1
|
7 | import os
8 | import urllib.parse
9 | from typing import Any, Literal, Optional, Union, List
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
10 | from collections.abc import Iterator, Mapping, Generator
11 | from pathlib import Path
|
UP006 Use `list` instead of `List` for type annotation
--> podman/domain/images_manager.py:175:27
|
173 | reference: Optional[str] = None,
174 | message: Optional[str] = None,
175 | changes: Optional[List[str]] = None,
| ^^^^
176 | ) -> "Image":
177 | """Import a tarball as an image (equivalent of 'podman import').
|
help: Replace with `list`
F841 Local variable `tar_path` is assigned to but never used
--> podman/tests/integration/test_images.py:327:13
|
325 | # Pack the testfile with the test folder in a tar buffer
326 | tar_buffer = io.BytesIO()
327 | tar_path = os.path.join(tmpdir, "test.tar.gz")
| ^^^^^^^^
328 | with tarfile.open(fileobj=tar_buffer, mode="w:gz") as tar:
329 | tar.add(base_dir, arcname="test")
|
help: Remove assignment to unused variable `tar_path`
Found 3 errors.
No fixes available (2 hidden fixes can be enabled with the `--unsafe-fixes` option).
15fa761 to
0cab523
Compare
I fixed the issues. |
podman/domain/images_manager.py
Outdated
| """Import a tarball as an image (equivalent of 'podman import'). | ||
|
|
||
| Args: | ||
| file_path: Path to a tarball (str) or a file-like object opened in binary mode. |
There was a problem hiding this comment.
I think description doesn't match file_path: Optional[os.PathLike] = None.
podman/domain/images_manager.py
Outdated
| response.raise_for_status() | ||
|
|
||
| body = response.json() | ||
| image_id = body.get("Id") or body.get("id") |
There was a problem hiding this comment.
yup, this should be handled for failure of getting Id
There was a problem hiding this comment.
I found out the true field name is "Id" so i will leave the other one out.
I guess i found a similar error handling as requested in the load function:
response.raise_for_status() # Catch any errors before proceeding
def _generator(body: dict) -> Generator[Image, None, None]:
# Iterate and yield images from response body
for item in body["Names"]:
yield self.get(item)
# Pass the response body to the generator
return _generator(response.json())
But being honest, I don't get what is happening here...
|
PTAL @inknos |
podman/domain/images_manager.py
Outdated
| data: Optional[bytes] = None, | ||
| file_path: Optional[os.PathLike] = None, | ||
| reference: Optional[str] = None, | ||
| message: Optional[str] = None, | ||
| changes: Optional[builtins.list[str]] = None, |
There was a problem hiding this comment.
I think I'd like to see only the required args as named, the others being kwargs. It's an arbitrary approach, but the library is designed this way, so I would follow this design pattern.
def import_image(
self,
data: Optional[bytes] = None,
file_path: Optional[os.PathLike] = None,
**kwargs,Kwargs being: reference, message, changes, and url, which is missing. Do you plan to implement it? if not, we'll have to document it with TODO and reference a new issue in the docstring.
There was a problem hiding this comment.
i fixed the style. reference, message and changes are implemented and tested (in the integration test).
The url parameter i did not implement (yet). Would be a solution with with urllib.request.urlopen(source) as response: ... ok for you?
There was a problem hiding this comment.
Or alternate with requests:
with requests.get('https://httpbin.org/get', stream=True) as r:
for chunk in r.iter_content(chunk_size=8192):
....
| params = {} | ||
| if reference: | ||
| params["reference"] = reference | ||
| if message: | ||
| params["message"] = message | ||
| if changes: | ||
| params["changes"] = changes # requests sends repeated keys as a list |
There was a problem hiding this comment.
Following the comment above, this can be simplified with the same logic of other functions that do kwargs.get()
f8a46e7 to
cb765a8
Compare
for importing raw tar files. This corresponds "podman image import <path> [reference]" on the command line Signed-off-by: Andre Wagner <[email protected]>
165cfdc to
b235fe9
Compare
|
Hi, Greetings, |
| params["url"] = url | ||
|
|
||
| # Get either from file or from raw data | ||
| post_data_context = Path(file_path).open("rb") if file_path else io.BytesIO(data) |
| with patch("pathlib.Path.open", return_value=io.BytesIO(b"mock tarball data")): | ||
| mock.post( | ||
| tests.LIBPOD_URL + "/images/import", | ||
| json={"Id": "quay.io/fedora:latest"}, |
There was a problem hiding this comment.
Will the actual import return an image tag or an image ID?
Hi @ALL,
while using podman-py i realized that there is no python-binding for podman import, so i created one by .myself.
I works straight forward:
I tested it successfully on Ubuntu 24.04 with podman 4.9.3 and on Ubuntu 26.04 snapshot 3 (snapshot 4 does not work on qemu for some reason) and podman 5.7.0.
What do you think?
Greetings,
André